Fix STAC writing and add AWS writing functions #53
Conversation
alexgleith
force-pushed
the
add-s3-writer
branch
from
295fb30 to
18698e2
Compare
|
Ok, rebased and cleaned up the commits. |
alexgleith
changed the title
dep_tools/writers.py
Outdated
| stac_writer = StacWriter(itempath, write_stac_function) if write_stac else None | ||
| super().__init__(itempath, pre_processor, cog_writer, stac_writer) | ||
| stac_writer = ( | ||
| StacWriter(itempath, write_stac_function, **kwargs) if write_stac else None |
There was a problem hiding this comment.
This (and below) is not going to work. We can't send the same kwargs to three places. I wrote DepWriter to deal with this situation. I was on the fence about writing this wrapper, maybe it wasn't the best idea
| @@ -102,8 +102,8 @@ def bbox_across_180(region: GeoDataFrame) -> BBOX | tuple[BBOX, BBOX]: | |||
| ) | |||
| if bbox_crosses_antimeridian: | |||
| # Split into two bboxes across the antimeridian | |||
| xmax_ll, ymin_ll = bbox[0], bbox[1] | |||
There was a problem hiding this comment.
if the bbox is e.g. [-179, -1, 179, 1] then -179 is east of 179 and therefore the xmax.
There was a problem hiding this comment.
Not saying it's perfect, can you say where this was causing issues?
There was a problem hiding this comment.
Bounding boxes are (in the tools we use) [minx, miny, maxx, maxy]
So we should remain consistent with that.
I think one of your tests had a geometry where effectively the bbox was world wrapping, so doing xmax_ll, ymin_ll = bbox[0], bbox[1] worked, but it failed on "normal" bboxes.
I tested all this a fair bit when I wrote it. I think including the antimeridian library makes it more robust too.
There was a problem hiding this comment.
This is in that format ([minx, miny, maxx, maxy]), in "real world" coordinates. It comes down to whether the smaller number is the minimum, or the western most point is the minimum. This selects the latter. We could write it either way.
Please show me an example of where this code fails. Your fix is not passing existing tests which the existing version worked for.
There was a problem hiding this comment.
if the bbox is e.g. [-179, -1, 179, 1] then -179 is east of 179 and therefore the xmax.
Ok, I see what you're saying, but I think this is the core of the issue, i.e., it's ambiguous. It could be a valid BBOX, which spans nearly the entire world, and it's really more valid to think of it as that, since the convention is to say minx, miny, maxx, maxy.
Does that make sense? It's either a bounding box going from far west to far east, or a bounding box going just across the antimeridian... and we should assume the former, I think.
There was a problem hiding this comment.
Hence my comment "these are pacific specific tests"...i.e. I was inferring the latter
tests/test_antimeridian_fixes.py
Outdated
| @@ -24,13 +23,14 @@ def test_shift_negative_longitudes_noncrossing_linestring(): | |||
|
|
|||
| def test_bbox_across_180_crossing(): | |||
| crossing_polygon = Polygon( | |||
| [(179.0, 1.0), (-179.0, 1.0), (-179.0, -1.0), (179.0, -1.0), (179.0, 1.0)] | |||
| [(179.0, 1.0), (179.0, -1.0), (-179.0, -1.0), (-179.0, 1.0), (179.0, 1.0)] | |||
There was a problem hiding this comment.
Reason for this change? I was trying to send something that was incorrectly wound (clockwise) I think
There was a problem hiding this comment.
The https://pypi.org/project/antimeridian/ package throws warnings for incorrectly wound polygons.
I don't think we should be handling bad geometry, we should just make sure we don't generate it.
Is the incorrect geometry something we need to handle?
There was a problem hiding this comment.
Yes, we should, because it's out there. Why not?
There was a problem hiding this comment.
Right, sure.
We do need to handle bad data, sure.
Let's do everything we possibly can to ensure we don't produce our own bad data, and then we don't need to write our own stuff that handles it! haha (Not saying we are creating bad data now, of course!)
|
Hey @jessjaco the only failing test now is this one:
And I haven't fixed it yet because I don't know what the correct answer is. Presumably the result should be |
It's from your own issue, as I commented in the code: microsoft/PlanetaryComputer#296 edit: Ok, I admit that 24 is a magic number not inferred from the issue, but this is the situation I was trying to repair. I welcome a more robust test. |
Sorry, not trying to blame here! Just saying I didn't fix it by making it |
Now that I look at it, why did you make it 24? |
Oh dear, it does appear I'm to blame! |
…tion of target grid
… of digits in fractional seconds
Refactor STAC writing, as I don't think it was working before. Was only writing a single
asset.Fixes
pathreturns from the writer.Also implements an S3 writer, including all the existence checks and so on.
I think this is ready for merging, although I could clean up the commits a bit!